-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ASAN/UBSAN recipe to just, make a CI that runs it + adding GDB #1474
Conversation
Thinking a bit more about this, do we need a separate sanitizer build? Why not just run all of the CI with a sanitized build? |
It takes forever... we usual test 10k events, I tried with 7.5k for the ecal pn, and was killed at 6 hours. I'm now converging to the point of only testing 1500 events which will take about an hour. (But for some reason I cant keep the ASAN output in the CI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One DRY comment that is optional and some guidance on writing the workflow.
This sounds really really strange. The sanitizers should have an extremely small impact on performance |
Hmm, I dont know, here is the action that was killed after 6 hours https://github.com/LDMX-Software/ldmx-sw/actions/runs/11007030465 if it helps |
Ok the output is being saved now |
(sorry I'm going back and forth between draft and ready -- that's my only way to trigger tests until it's merged [after that we'll be able to do this from the Actions with the dispatch options]) |
Hmm, I dont understand, in local the ASAN output is in stderr. But in the action I only get https://github.com/LDMX-Software/ldmx-sw/actions/runs/11044190876
even tho |
That seems strange... But the simulation finishes and the leak sanitizer issue is only at the end? I'm still a bit skeptical to the value of running leaksanitizer in CI for us so I might just turn it off isntead |
I'm running ASAN + UBSAN, and they somehow invoke LSAN too. And yes, they only show up in the end, bc of the setting to dont stop on issues (which is a default set by you @EinarElen I believe, but correct me if I'm wrong [given that we fixed all the issues, it's hard to tell]). I can be convinced either way about the leak part. But the issue about github dealing with stderr from ASAN/UBSAN is not going to be connected to the leak part. Maybe @tomeichlersmith has an idea about the environment settings on the machines used by the actions. |
The runners do not swallow anything from stdout or stderr, but they do not have a TTY and so programs that are "smart" and disable certain behaviors in a non-interactive environment (mainly determined by detecting if a TTY is present) will disable those behaviors. You can mimic running without a TTY to see if that is the issue. |
LSAN is a part of ASAN so it gets enabled by default. You turn if off with ASAN_OPTIONS=detect_leaks=0 |
so none of this managed to reproduce what I see in the actions, but I think I found a workaround with specifying the ASAN output, I'll test that now |
OK I think I'm ready now, this is the module operandi I propose: we dont save the log for now (it didnt work even with specifying the output for ASAN), we dont run the leak part --> this means currently there is no issues. Then we run this every time a new PR comes in, and if any of those introduce a problem, this test will fail. It will not show what the issue is (given the output issue), but it will show that there is an issue, so we can check out the branch locally and run this and we'll see the problem. |
Co-authored-by: Tom Eichlersmith <31970302+tomeichlersmith@users.noreply.github.com>
79c2089
to
b118343
Compare
OK, tests are fine, @tomeichlersmith @EinarElen I wont push anymore, please approve if you agree with the changes |
I am updating ldmx-sw, here are the details.
What are the issues that this addresses?
Resolves #1043 (the last bit of it)
Check List
I did
give a lot of indirect memory leaks, which is fine, none of them are big. This is kinda expected since we fixed the ASAN / UBSAN problems in previous PRs.
Added it to the CI with the option w/o the leak sanitizer, so currently there is no output from ASAN/UBSAN (given that we cleaned all the issues up already).
Then
this runs too, although I'm not sure how to actually make use of it for a real config file with
fire
.I introduced
as well.